-
-
Notifications
You must be signed in to change notification settings - Fork 32
ci: add e2e tests to CI #546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This change adds a new end-to-end test capability. There are two new jobs in the main CI workflow. One job to build the package (which also good to have outside of the need to run e2e tests), executed with the latest LTS node. The build artifact is uploaded for the downstream `e2e` job to use. The `e2e` job, downloads the artifact, run `npm install` and then `npm run e2e`, which executes a custom node script to loop through subdirectories of `/e2e`, which each contain a project fixture with different setups. Both projects have the same simple plugin source code, and each has this plugin (referenced from the root of the repo (aka the result of the build)) and `eslint` installed as `devDependencies`. Each also has a `lint` package.json script that runs `eslint` on the simple plugin. \## `/e2e/all` This fixture has a regular js config and runs our `all` config on the project. \## `/e2e/all-typed-config` This fixture has `typescript` installed and uses a ts-based config and runs our `all` config on the project. It also executes `tsc` as part of the `lint` command, to ensure everything's typed property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat.
I also ran this through AI if you'd like to have a look at what it thought: https://chatgpt.com/share/68993709-d248-800c-a59f-c765c9b7b043
- '20.19.0' #minimum supported v20 | ||
- 20 | ||
os: | ||
- ubuntu-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we may be missing tests on Windows. Should we test that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can add that. Do you want it added to the unit test matrix too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to the e2e job. If you want me to add it to the test job, I can do that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally would be good to test on both. Hopefully it doesn't hurt perf too much. Also could be done separately.
e2e/runAllTests.js
Outdated
/** | ||
* Run All Tests: This script executes the lint command on all subfolders under `fixtures`, in order | ||
* to validate the correctness of our plugin. Each fixture installs the *built* package. So this should | ||
* only be run after a build has been done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, how is the performance impact of this on our builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In total, it adds around 40s to a minute to the overall CI runtime. Mainly because the E2E steps have to wait for the build to complete. Just looking at the build readout below, it took 38s for the build job, which ran in parallel to all of the other existing jobs. At that point all the E2E jobs kicked off in parallel, and the longest running of those are the windows ones, which took about a minute each (the ubuntu ones took between 40-50s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE: performance, though, there will be a roughly linear impact to the length of those steps, for each e2e test that's added in the future. So, one thing to consider, if many more are added, would be to run some number of them concurrently. I wouldn't necessarily go to that right now, because it adds some amount of complexity. With just two tests, there's not much to gain with doing them concurrently. But if there were 10 or more, it might start to make sense.
Btw, I'm thinking it might be a good idea to move the prettier config back to js, until the VSCode extension supports it. prettier/prettier-vscode#3623 IDE-autoformatting isn't working anymore. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Up to you on that. If it's going to be broken for a while, then could make sense to revert. If it's going to be fixed soonish, then could wait. |
This change adds a new end-to-end test capability. There are two new jobs in the main CI workflow. One job to build the package (which is also good to have outside of the need to run e2e tests), executed with the latest LTS node. The build artifact is uploaded for the downstream
e2e
job to use. Thee2e
job, downloads the artifact, runnpm install
and thennpm run e2e
, which executes a custom node script to loop through subdirectories of/e2e
, which each contain a project fixture with different setups. Both projects have the same simple plugin source code, and each has this plugin (referenced from the root of the repo (aka the result of the build)) andeslint
installed asdevDependencies
. Each also has alint
package.json script that runseslint
on the simple plugin./e2e/all
This fixture has a regular js config and runs our
all
config on the project./e2e/all-typed-config
This fixture has
typescript
installed and uses a ts-based config and runs ourall
config on the project. It also executestsc
as part of thelint
command, to ensure everything's typed property.